Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

VT sequence support for EraseInLine, EraseInDisplay, DeleteCharacter and InsertCharacter #2144

Merged
merged 5 commits into from
Jul 30, 2019
Merged

VT sequence support for EraseInLine, EraseInDisplay, DeleteCharacter and InsertCharacter #2144

merged 5 commits into from
Jul 30, 2019

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Jul 29, 2019 •
edited by DHowett-MSFT
Loading

Summary of the Pull Request

Implemented the following VT sequences:

  • EraseInLine
  • EraseInDisplay
  • DeleteCharacter
  • InsertCharacter

(Minor: implemented CursorUp and CursorBackward in TerminalDispatch with existing APIs)

References

Related to #1883

PR Checklist

  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Without these VT sequences, non-Conhost connections behave strangely (since they do not go through conpty for VT parsing).

Validation Steps Performed

Compared behaviour with putty/other terminals and imitated them

@DHowett-MSFT
Copy link
Contributor

nit: title. rephrase to be something imperative, as though you were completing the sentence:

if merged, this pull request will ...

zadjii-msft
zadjii-msft requested changes Jul 30, 2019
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to see some tests added for these guys, but I understand that there's probably not time to add them now.
I've got a couple nit-level questions I want answered, but otherwise great work!

src/cascadia/TerminalCore/Terminal.hpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 30, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 30, 2019
@PankajBhojwani PankajBhojwani changed the title Vt sequences VT sequence support for EraseInLine, EraseInDisplay and DeleteCharacter Jul 30, 2019
miniksa
miniksa requested changes Jul 30, 2019
View reviewed changes
src/buffer/out/textBuffer.cpp Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Show resolved Hide resolved
src/cascadia/TerminalCore/TerminalApi.cpp Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 30, 2019
zadjii-msft
zadjii-msft approved these changes Jul 30, 2019
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright I'm happy with this

@PankajBhojwani PankajBhojwani changed the title VT sequence support for EraseInLine, EraseInDisplay and DeleteCharacter VT sequence support for EraseInLine, EraseInDisplay, DeleteCharacter and InsertCharacter Jul 30, 2019
miniksa
miniksa approved these changes Jul 30, 2019
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

DHowett-MSFT
DHowett-MSFT approved these changes Jul 30, 2019
View reviewed changes
src/buffer/out/textBuffer.cpp
while (fDoBackUp)
{
coordEndOfText.Y--;
pCurrRow = &GetRowByOffset(coordEndOfText.Y);
// We need to back up to the previous row if this line is empty, AND there are more rows

coordEndOfText.X = static_cast<short>(pCurrRow->GetCharRow().MeasureRight()) - 1;
fDoBackUp = (coordEndOfText.X < 0 && coordEndOfText.Y > 0);
fDoBackUp = (coordEndOfText.X < 0 && coordEndOfText.Y > viewportTop);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, should this make sure that X is within viewportRight? Or something equivalent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like X will be -1 if there was no non-space text in the row (based on what MeasureRight) is doing, so I don't think its relative to where the viewport is

src/cascadia/TerminalCore/TerminalApi.cpp Show resolved Hide resolved
@PankajBhojwani PankajBhojwani merged commit 63df881 into microsoft:master Jul 30, 2019
@PankajBhojwani PankajBhojwani deleted the VTSeq branch July 30, 2019 23:28
@j4james j4james mentioned this pull request Jul 31, 2019
Closed
DHowett-MSFT pushed a commit that referenced this pull request Aug 6, 2019
@DHowett
VT sequence support for EraseInLine, EraseInDisplay, DeleteCharacter …
ddd413c 
…and InsertCharacter (#2144)

* We now support EraseInLine, EraseInDisplay, DeleteCharacter and InsertCharacter

(cherry picked from commit 63df881)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlos-zamora carlos-zamora carlos-zamora left review comments

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

@miniksa miniksa miniksa approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@PankajBhojwani @DHowett-MSFT @carlos-zamora @miniksa @zadjii-msft